planner: arm engine-restricted alternative logical plan rounds from round 1's engine usage (WIP) - #70020
planner: arm engine-restricted alternative logical plan rounds from round 1's engine usage (WIP)#70020terry1purcell wants to merge 2 commits into
Conversation
Adds tikv-only and tiflash-only alternative logical plan rounds that arm from the storage engines round 1's chosen plan actually read, rather than requiring that plan to mix engines. An all-TiKV plan can still hide a better whole-statement MPP plan: the bottom-up search picks an engine per DataSource through local cost comparisons, so per-table index access can win table by table even when a homogeneous TiFlash plan would win overall, and that alternative is otherwise never costed. InspectPlanShape walks round 1's plan for engine usage and join/agg presence, descending into reader subtrees because pushed-down aggregations and MPP joins live inside cop and MPP tasks. Gates keep the extra rounds off statements that cannot benefit: a join or aggregation must be present, no READ_FROM_STORAGE hint may pin an engine, enforced MPP is excluded (its cost discount would distort the cross-round comparison), and tiflash-only additionally requires every table to have a TiFlash access path and MPP to be allowed. This is an alternative design to pingcap#70005; only one of the two will ship. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvP1rNMDfN8B6S7jx14Y7M
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
sig/planner
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
BenchmarkEngineRoundOptimize measures per-statement optimization time for the alternative logical plan driver on the query class where this design and the mixed-engine-only design (pingcap#70005) diverge: an all-TiKV join that this branch arms the tiflash-only round for. A scan-only case confirms the join/agg gate leaves the OLTP fast path untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvP1rNMDfN8B6S7jx14Y7M
ti-chi-bot
Bot
added
size/XXL
|
/test all |
There was a problem hiding this comment.
Pull request overview
This PR adjusts TiDB’s alternative logical plan driver to cost engine-homogeneous alternatives (TiKV-only / TiFlash-only) based on the actual engine usage and join/aggregation presence in round 1’s chosen physical plan, so that even an all-TiKV plan can still be compared against a whole-statement TiFlash/MPP alternative.
Changes:
- Add
physicalop.InspectPlanShapeto detect (from round 1’s physical plan) whether TiKV/TiFlash was read and whether a join/aggregation exists. - Record new round-arming signals in
StatementContext, and use them to gate newtikv-only/tiflash-onlyalternative rounds. - Add casetests and a benchmark covering arming/gating behavior and validating session state restoration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sessionctx/stmtctx/stmtctx.go | Adds new AlternativeLogicalPlan* signals for engine usage, join/agg presence, missing TiFlash paths, and store-type hints. |
| pkg/planner/optimize.go | Uses InspectPlanShape to arm new engine-restricted rounds; refactors per-round setup/cleanup into closures to avoid cross-session clobbering. |
| pkg/planner/core/operator/physicalop/plan_shape.go | Introduces PlanShape and InspectPlanShape to walk physical plans (including pushed-down subtrees) for engine usage and join/agg detection. |
| pkg/planner/core/operator/physicalop/BUILD.bazel | Adds plan_shape.go to the Bazel target sources. |
| pkg/planner/core/logical_plan_builder.go | Marks store-type hint usage and records missing-TiFlash-path signal during DataSource build. |
| pkg/planner/core/casetest/mpp/engine_rounds_test.go | Adds unit test covering arming/gating of tikv-only / tiflash-only rounds and state restoration. |
| pkg/planner/core/casetest/mpp/engine_rounds_bench_test.go | Adds a benchmark to compare optimization overhead with the feature off vs on for representative queries. |
| pkg/planner/core/casetest/mpp/BUILD.bazel | Registers new test/bench files and updates shard count/deps. |
| pkg/planner/BUILD.bazel | Adds dependency on physicalop due to new InspectPlanShape usage from planner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func shouldTryTiKVOnlyRound(sessVars *variable.SessionVars) bool { | ||
| return shouldTryEngineRestrictedRounds(sessVars) && | ||
| sessVars.StmtCtx.AlternativeLogicalPlanReadsFromTiFlash | ||
| } |
| func shouldTryTiFlashOnlyRound(sessVars *variable.SessionVars) bool { | ||
| return shouldTryEngineRestrictedRounds(sessVars) && | ||
| sessVars.StmtCtx.AlternativeLogicalPlanReadsFromTiKV && | ||
| !sessVars.StmtCtx.AlternativeLogicalPlanMissingTiFlashPath && | ||
| sessVars.IsMPPAllowed() | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70020 +/- ##
================================================
- Coverage 76.3233% 73.9089% -2.4145%
================================================
Files 2041 2059 +18
Lines 559759 579340 +19581
================================================
+ Hits 427227 428184 +957
- Misses 131631 150792 +19161
+ Partials 901 364 -537
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This is an alternative design to #70005; only one of the two will ship.
The physical search picks a storage engine per DataSource through local cost comparisons, so per-table index access can win table by table even when a homogeneous whole-statement plan (in particular an MPP plan) would win overall — and that homogeneous alternative is never costed as a unit. #70005 arms the tikv-only / tiflash-only rebuild rounds only when round 1's chosen plan already mixes both engines. This PR instead arms each round from the engines round 1 actually read, so an all-TiKV plan still gets a whole-statement TiFlash/MPP alternative costed against it — the case #70005 structurally cannot reach.
InspectPlanShapewalks round 1's chosen plan once for its engine usage and for whether it joins or aggregates, descending into reader subtrees because pushed-down aggregations and MPP joins live inside cop and MPP tasks. The tikv-only round arms when round 1 read from TiFlash anywhere; the tiflash-only round arms when it read from TiKV anywhere, including the all-TiKV case. Shared gates keep the extra rounds off statements that cannot benefit: a join or aggregation must be present (keeps the rounds off the OLTP fast path), no READ_FROM_STORAGE hint may pin an engine, and enforced MPP is excluded (its cost discount would distort the cross-round comparison). The tiflash-only round additionally requires every table to have a TiFlash access path and MPP to be allowed.What problem does this PR solve?
Issue Number: ref #70021
Problem Summary:
The alternative logical plan driver only builds a fully engine-homogeneous plan when round 1 already mixed engines. An all-TiKV plan can still hide a cheaper whole-statement MPP plan that was never costed, and that case is not covered.
What changed and how does it work?
physicalop.InspectPlanShapereturns, from a single walk of round 1's chosen plan, which storage engines it read from and whether it contains a join or aggregation. It descends into reader subtrees (TablePlan / IndexPlan / partial plans) because pushed-down aggregations and MPP joins live inside cop and MPP tasks.optimizerecords those facts as StmtCtx signals after round 1 and pre-computes round eligibility from them (so later rounds' side-effect signals cannot leak into arming).tikv-onlyandtiflash-only, rebuild the plan withIsolationReadEnginesrestricted to a single storage engine; the existing strict-<cross-round cost comparison picks the winner. Arming and gates are described above.setupnow returns its own cleanup closure instead of using package-level saved state, so concurrentoptimizecalls in different sessions cannot clobber one another's saved values.Check List
Tests
TestAlternativeEngineRoundArminginpkg/planner/core/casetest/mpp/engine_rounds_test.gocovers each arming and gating case, including the headline all-TiKV-plan-arms-tiflash-only case, the missing-replica skip, the join/agg gate, the hint gate, enforced MPP, and the feature-off case.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
🤖 Generated with Claude Code
https://claude.ai/code/session_01TvP1rNMDfN8B6S7jx14Y7M